Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

added --debug-lookup to log library loading #6436

Merged
merged 1 commit into from
Jun 11, 2024
Merged

added --debug-lookup to log library loading #6436

merged 1 commit into from
Jun 11, 2024

Conversation

firewave
Copy link
Collaborator

No description provided.

@firewave
Copy link
Collaborator Author

A few thoughts:

  • we are putting a hard-coded path in the binary and nowhere it is visible to the user. We should have a way to expose this so it can be used by the test as well as viewed by others. e.g. --show-filesdir
  • should we make it possible to disable the usage of the built-in path (because it is hard-coded and cannot be changed by the user)? e.g. --disable-filesdir
  • we should document which paths are used for looking up the libraries (and all other files). I have not checked yet if it already is.

@firewave
Copy link
Collaborator Author

Only the sanitizer builds fail so we can simply disable FILESDIR in those jobs.

@firewave
Copy link
Collaborator Author

FILESDIR cannot be overridden in a CMake build right now and is always set by default. In a Makefile it is not set by default and can be overriden.

@firewave firewave marked this pull request as draft May 24, 2024 07:51
@firewave firewave mentioned this pull request May 25, 2024
Merged
@firewave firewave marked this pull request as ready for review May 26, 2024 19:16
@danmar
Copy link
Owner

danmar commented May 30, 2024

FILESDIR cannot be overridden in a CMake build right now and is always set by default.

sounds like a "bug" hope that can be fixed. the intention is that it will only be set if the package maintainer wants to set a hardcoded path. For instance in windows it should not be set. In my humble opinion --disable-filesdir does not sound related it is the package installation script duty to install cppcheck properly

@firewave
Copy link
Collaborator Author

FILESDIR cannot be overridden in a CMake build right now and is always set by default.

sounds like a "bug" hope that can be fixed.

That was already fixed by #6437.

the intention is that it will only be set if the package maintainer wants to set a hardcoded path. For instance in windows it should not be set.

We do set that for the official packages.

In my humble opinion --disable-filesdir does not sound related it is the package installation script duty to install cppcheck properly

This is more a thing about safety as this hard-coded path (currently also invisible to the user) cannot be controlled. It should be the last one in the lookup order so it is basically just a fall-through but still.

But none of this is scope of this PR. That should be tracked via tickets.

@firewave
Copy link
Collaborator Author

firewave commented Jun 5, 2024

So can this be merged? This is the required baseline for fixing issues/doing cleanups.

@firewave
Copy link
Collaborator Author

firewave commented Jun 5, 2024

I filed https://trac.cppcheck.net/ticket/12810 and https://trac.cppcheck.net/ticket/12811 about adding the proposed options.

danmar
danmar reviewed Jun 9, 2024
View reviewed changes
lib/library.cpp Show resolved Hide resolved
gui/mainwindow.cpp Show resolved Hide resolved
@firewave firewave changed the title added verbose logging of library loading added --debug-lookup to log library loading Jun 11, 2024
danmar
danmar approved these changes Jun 11, 2024
View reviewed changes
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good.. I am not against this --debug-lookup. I just have a question.

.github/workflows/asan.yml
@@ -69,7 +69,7 @@ jobs:
# TODO: disable all warnings
- name: CMake
run: |
cmake -S . -B cmake.output -DCMAKE_BUILD_TYPE=RelWithDebInfo -DHAVE_RULES=On -DBUILD_TESTS=On -DBUILD_GUI=Off -DWITH_QCHART=Off -DUSE_MATCHCOMPILER=Verify -DANALYZE_ADDRESS=On -DENABLE_CHECK_INTERNAL=On -DUSE_BOOST=On -DCPPCHK_GLIBCXX_DEBUG=Off -DCMAKE_DISABLE_PRECOMPILE_HEADERS=On -DCMAKE_GLOBAL_AUTOGEN_TARGET=Off -DDISABLE_DMAKE=On -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache
cmake -S . -B cmake.output -DCMAKE_BUILD_TYPE=RelWithDebInfo -DHAVE_RULES=On -DBUILD_TESTS=On -DBUILD_GUI=Off -DWITH_QCHART=Off -DUSE_MATCHCOMPILER=Verify -DANALYZE_ADDRESS=On -DENABLE_CHECK_INTERNAL=On -DUSE_BOOST=On -DCPPCHK_GLIBCXX_DEBUG=Off -DCMAKE_DISABLE_PRECOMPILE_HEADERS=On -DCMAKE_GLOBAL_AUTOGEN_TARGET=Off -DDISABLE_DMAKE=On -DFILESDIR= -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason to define FILESDIR here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Because it is set by default in CMake but not with make the tests will only work on either. For now I have omitted it from testing. This is one of the reasons an option to disable the usage of the built-in path would make sense.

@firewave firewave merged commit 20472f5 into danmar:main Jun 11, 2024
63 checks passed
@firewave firewave deleted the lib_impl-3 branch June 11, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danmar danmar danmar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@firewave @danmar